Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

pr-reviewer could not complete review for #84 in AgentWorkforce/agents.
The review harness exited with code 1.
No review was posted; this needs operator attention.

ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed.

PR #84 Review — fix/joke-bot-channel-guard

Summary

This PR makes three coordinated changes to joke-bot:

1. agent.ts — adds a channel guard in handleSlackMention that only replies in the configured SLACK_CHANNEL, normalizing the incoming id__name → id.
2. persona.ts — flips sandbox: true → sandbox: false, with a comment explaining the Slack writeback now routes over the relayfile HTTP API (no FS mount needed).
3. package.json / package-lock.json — bumps @relayfile/relay-helpers 0.3.42 → ^0.4.1 (pulls adapter-core 0.4.2), the dependency that enables the mount-less HTTP writeback.

The three changes are internally consistent: sandbox:false removes the relayfile FS mount, and the relay-helpers ≥0.4.1 bump is what lets writeJsonFile fall back to the HTTP client when no mount exists.

Verification (run against the current checkout)

• npm ci — succeeds → package.json and package-lock.json are in sync.
• npm run typecheck (tsc --noEmit) — passes.
• npm test (tsc build + node --test tests/*.test.mjs) — 125/125 pass when run in a clean environment.
• agentworkforce persona compile joke-bot/persona.ts — compiles cleanly with sandbox:false.

Note on a transient test failure: in this harness sandbox, tests/hn-monitor.test.mjs initially failed because the harness injects process.env.SLACK_CHANNEL=C0ALQ06AAUT (the reviewer agent's own deploy context), and hn-monitor's input() helper prefers process.env over persona inputs, overriding the test's 'C123'. Re-running with that env var unset (env -u SLACK_CHANNEL npm test) yields 125/125 green. This is a sandbox environment artifact, not a PR regression — hn-monitor is untouched by this PR and real CI has no such injected var. No code change made.

Findings (left as comments — not auto-edited)

1. Channel guard: asymmetric normalization of want vs chanId — joke-bot/agent.ts:143-145

const want = input(ctx, 'SLACK_CHANNEL');
const chanId = channel.split('__')[0];
if (want && chanId !== want) { ... return; }

The incoming channel is normalized (split('__')[0]) but want is not. If the SLACK_CHANNEL picker ever resolves to an id__name form (the same form being defended against on the inbound side), want would carry the __name suffix and chanId !== want would always be true, causing the bot to silently never reply. The current evidence (capabilities.channels: ['C0AD7UU0J1G'] is a bare id, and paths: ['/slack/channels/${SLACK_CHANNEL}/**']) suggests want is a bare id today, so this works — but the normalization is one-sided. Consider normalizing want the same way (input(...)?.split('__')[0]) for symmetry/robustness. This is a fail-safe (skip on mismatch) behavior decision, so I'm leaving it for human judgment rather than editing.

2. Stale module docstring in persona.ts — joke-bot/persona.ts:14-19
The top-of-file doc comment still describes the old design: "It is also the harness test vehicle: the joke is generated via ctx.harness.run (NOT ctx.llm.complete)" and "Reply surface is Slack writeback (slackClient().post)". This now contradicts the changed code — the handler uses ctx.llm.complete and runs with sandbox:false. The inline comment at lines 29-34 was updated, but this header block was not. Comment-only (no runtime effect); recommend the author align the header prose with the new behavior. Left unchanged to keep this review scoped to the diff and avoid prose rewrites.

Addressed comments

• No bot or human review comments were present in the provided PR metadata (.workforce/context.json contains no comments/reviews, and there is no comments file). Nothing to address.

Advisory Notes

• The overrides block in package.json pins @relayfile/adapter-linear to 0.3.11, while the new relay-helpers@0.4.1 declares @relayfile/adapter-linear: ^0.4.1. npm ci resolves cleanly (the override wins, installing 0.3.11), so this is not a build break and it predates this PR (the old relay-helpers@0.3.42 required ^0.3.23 and was likewise overridden to 0.3.11). Out of scope for this PR; flagging only so a maintainer can confirm the 0.3.11 pin is still intended now that the relay-helpers line has moved to 0.4.x.

Assessment

The diff is coherent, minimal, and verified: typecheck passes, the full test suite passes in a clean environment, deps are in sync, and the persona compiles. No mechanical fixes were required, so nothing was auto-edited. The two findings above are behavior/judgment items (a robustness suggestion and a stale-comment cleanup) for the author to weigh — neither blocks the PR's stated purpose.

I am not printing READY: I cannot observe the actual GitHub CI check states or mergeability from this sandbox (no gh/git access, and the only test failure I saw was a sandbox env artifact). Confirming all required checks are green and the PR is mergeable is a post-harness step; the findings above are advisory and do not require code changes from me.

ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed.

Working tree is clean — I made no edits (none were needed). The .test-build dir is removed by the test script, and node_modules is gitignored.

Review Summary

PR #84 (fix/joke-bot-channel-guard) makes three coherent changes to the joke-bot agent:

1. Channel guard in handleSlackMention (joke-bot/agent.ts:139-154) — adds a fail-closed check so the bot only replies in the configured SLACK_CHANNEL, normalizing id__name → id via channel.split('__')[0]. This is correct and safety-positive: it preserves fail-closed semantics (returns without replying when SLACK_CHANNEL is unset), matching the relay/cron paths. No callers or types are affected.

2. sandbox: true → false (joke-bot/persona.ts:34) — relies on relay-helpers ≥0.4.1 routing writeJsonFile over the RelayFile HTTP API instead of the FS mount, so no Daytona box is needed. This is a deliberate behavior change backed by the dependency bump; the rationale is documented.

3. @relayfile/relay-helpers 0.3.42 → ^0.4.1 (package.json, package-lock.json) — lockfile is internally consistent (npm ci succeeds; nested adapter-core@0.4.2 recorded; the adapter-linear@0.3.11 override resolves cleanly).

Verification: npm ci ✓, npm run typecheck (tsc --noEmit) ✓, npm test → 125/125 pass in a clean environment.

Addressed comments

• No bot or human review comments were present (.workforce/ contains only pr.diff, changed-files.txt, and context.json; no comments file). Nothing to address.

Advisory Notes

• Stale persona doc header (joke-bot/persona.ts:14-19): the file-level comment still states the joke is generated "via ctx.harness.run (NOT ctx.llm.complete)" and references the old sandbox:true rationale, which now contradicts both the updated agent.ts header (lines 10-15) and this PR's sandbox:false + ctx.llm.complete reality. The agent.ts header was updated; persona.ts was not. Left unchanged because it concerns the persona's stated harness-test purpose (human-judgment wording, not a mechanical typo). Recommend a follow-up to align the persona.ts doc comment with the new behavior.
• Test failure observed in this sandbox only (tests/hn-monitor.test.mjs:111): the test fails here because the sandbox sets SLACK_CHANNEL=C0ALQ06AAUT in the process env, and hn-monitor's input() (hn-monitor/agent.ts:243) prioritizes process.env over the test-injected persona.inputs. This is environment leakage in the sandbox, not a regression from this PR (the PR touches neither hn-monitor/ nor @agentworkforce/runtime). With the env var unset — as in CI — all 125 tests pass. No change made; flagging in case the hn-monitor test's env-precedence brittleness is worth a separate fix.

The PR is internally consistent, typechecks, and its tests pass in a clean environment. No mechanical or semantic fixes were required, so the working tree is unchanged. The remaining items (the sandbox:false behavior change correctness and the advisory doc/test notes) are human-judgment calls, and I cannot confirm the live CI check status or PR mergeability from here.

ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed.

Review: PR #84 — fix(joke-bot): channel guard + sandbox:false + relay-helpers 0.4.1

Summary of changes

The PR makes three related changes to joke-bot:

1. Channel guard in handleSlackMention (joke-bot/agent.ts:139-154) — fail-closed if SLACK_CHANNEL unset; skip if the event channel ≠ configured channel.
2. sandbox: true → sandbox: false (joke-bot/persona.ts:34) with a rewritten justification comment, relying on the new HTTP writeback path.
3. Dependency bump @relayfile/relay-helpers 0.3.42 → ^0.4.1 (package.json, package-lock.json).

Verification performed

• npm ci — clean (lockfile and package.json are in sync; the overrides block is honored).
• npm run typecheck (tsc --noEmit) — clean.
• npm test (full suite, the canonical command) — 125/125 pass under CI-equivalent conditions.
• One initial failure in tests/hn-monitor.test.mjs was reproduced and traced to a sandbox-only artifact: this reviewer environment exports SLACK_CHANNEL=C0ALQ06AAUT, and input() reads process.env before the persona input, shadowing the test's 'C123'. Unsetting it (as CI does) makes the suite fully green. Not a PR regression; no code or test change made.

No auto-edits were applied — the PR needed no mechanical (lint/format/typo/import) fixes, and the working tree is clean.

Findings (review comments — left for human judgment, not auto-edited)

1. joke-bot/agent.ts:146 — channel normalization is asymmetric. The guard normalizes only the event channel (chanId = channel.split('__')[0]) but compares it against the raw want = input(ctx, 'SLACK_CHANNEL'). If the picker ever resolves SLACK_CHANNEL to an id__name form (the comment on :144 explicitly anticipates that form exists on the event side), the comparison chanId !== want becomes "C123" !== "C123__general" and would silently drop legitimate mentions. This is fail-closed (no safety regression — correctly does not reply to the wrong channel), so I have not changed it. Suggested direction for the author: normalize both sides, e.g. compare chanId against want.split('__')[0]. The sibling agent linear-slack/agent.ts:154 compares raw-to-raw with no split and works, and the persona's own capabilities.channels uses a bare id (C0AD7UU0J1G), so confirm which form SLACK_CHANNEL actually resolves to before changing. Behavioral — needs the author's knowledge of the picker output.

2. sandbox: false is a runtime/architecture change (persona.ts:34). Correctness depends on relay-helpers ≥0.4.1 / adapter-core ≥0.4.2 actually routing writeJsonFile to RelayFileClient (HTTP) when no FS mount exists, using injected RELAYFILE_TOKEN/URL. That path can't be exercised in this sandbox. This is the central purpose of the PR and a human deploy-verification item — flagging rather than touching.

Advisory Notes

• Stale persona doc-comment (out of scope). joke-bot/persona.ts:14-19 still states the joke is generated via ctx.harness.run (NOT ctx.llm.complete) and "Reply surface is Slack writeback (slackClient().post)", but agent.ts uses ctx.llm.complete. This staleness predates this PR (the diff only touched the sandbox block), so I left it unchanged to avoid folding unrelated cleanup into this PR. Worth a follow-up doc fix.
• overrides vs. new dep tree (out of scope). package.json:30 pins @relayfile/adapter-linear to 0.3.11, while the new relay-helpers@0.4.1 declares @relayfile/adapter-linear: ^0.4.1 (lock :3941). npm ci resolves cleanly because the override forces 0.3.11, but if relay-helpers 0.4.x depends on adapter-linear 0.4.x API this could surface at runtime for Linear-using agents. joke-bot doesn't use Linear, and revisiting the override touches other packages — out of scope for this PR. Flagging for a maintainer.

Addressed comments

• No bot or human review comments were available in the harness payload (.workforce/context.json contains only repo/PR/SHA metadata; no reviews/comments fields, and .workforce/ has no comments file). Nothing to reconcile against the current checkout.

The PR is mechanically clean and passes the full build/typecheck/test suite locally, but it carries two behavioral items (the asymmetric channel normalization and the unverifiable sandbox:false HTTP-writeback path) that require author/deployer judgment, and I cannot confirm GitHub merge/CI status from here. I am not printing READY.